Simplify Bootstrap logic in tests #7252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 2, 2020 13:47
This change updates tests to honor `BootstrapExpect` exclusively when forming test clusters and removes test only knobs, e.g. `config.DevDisableBootstrap`. Background: Test cluster creation is fragile. Test servers don't follow the BootstapExpected route like production clusters. Instead they start as single node clusters and then get rejoin and may risk causing brain split or other test flakiness. The test framework expose few knobs to control those (e.g. `config.DevDisableBootstrap` and `config.Bootstrap`) that control whether a server should bootstrap the cluster. These flags are confusing and it's unclear when to use: their usage in multi-node cluster isn't properly documented. Furthermore, they have some bad side-effects as they don't control Raft library: If `config.DevDisableBootstrap` is true, the test server may not immediately attempt to bootstrap a cluster, but after an election timeout (~50ms), Raft may force a leadership election and win it (with only one vote) and cause a split brain. The knobs are also confusing as Bootstrap is an overloaded term. In BootstrapExpect, we refer to bootstrapping the cluster only after N servers are connected. But in tests and the knobs above, it refers to whether the server is a single node cluster and shouldn't wait for any other server. Changes: This commit makes two changes: First, it relies on `BootstrapExpected` instead of `Bootstrap` and/or `DevMode` flags. This change is relatively trivial. Introduce a `Bootstrapped` flag to track if the cluster is bootstrapped. This allows us to keep `BootstrapExpected` immutable. Previously, the flag was a config value but it gets set to 0 after cluster bootstrap completes.
The tests only care if a test server recognizes the leader.
notnoop
commented
Mar 3, 2020
| @@ -446,7 +433,7 @@ func TestNomad_BadExpect(t *testing.T) { | |||
| testutil.WaitForResult(func() (bool, error) { | |||
| for _, s := range servers { | |||
| p, _ := s.numPeers() | |||
| if p != 1 { | |||
| if p != 0 { | |||
There was a problem hiding this comment.
This was a change in behavior. When using bootstrap, the peers list remains empty until cluster is fully bootstrapped. Previously, servers will at least recognize themselves as a peer.
langmartin
approved these changes
Mar 3, 2020
nomad/config.go
Outdated
| // required so that it can elect a leader without any other nodes | ||
| // being present | ||
| Bootstrap bool | ||
| // Bootstrapped indictes if Server has bootstrapped or not. |
tgross
added a commit
that referenced
this pull request
Mar 4, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
tgross
added a commit
that referenced
this pull request
Mar 4, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
tgross
added a commit
that referenced
this pull request
Mar 9, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
langmartin
pushed a commit
that referenced
this pull request
Mar 13, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
tgross
added a commit
that referenced
this pull request
Mar 16, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
tgross
added a commit
that referenced
this pull request
Mar 23, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
tgross
added a commit
that referenced
this pull request
Mar 23, 2020
In #7252 we removed the `DevDisableBootstrap` flag to require tests to honor only `BootstrapExpect`, in order to reduce a source of test flakiness. This changeset applies the same fix to the CSI tests.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change updates tests to honor
BootstrapExpectexclusively whenforming test clusters and removes test only knobs, e.g.
config.DevDisableBootstrap.Background
Test cluster creation is fragile. Test servers don't follow the
BootstapExpected route like production clusters. Instead they start as
single node clusters and then get rejoin and may risk causing brain
split or other test flakiness.
The test framework expose few knobs to control those (e.g.
config.DevDisableBootstrapandconfig.Bootstrap) that controlwhether a server should bootstrap the cluster. These flags are
confusing and it's unclear when to use: their usage in multi-node
cluster isn't properly documented. Furthermore, they have some bad
side-effects as they don't control Raft library: If
config.DevDisableBootstrapis true, the test server may notimmediately attempt to bootstrap a cluster, but after an election
timeout (~50ms), Raft may force a leadership election and win it (with
only one vote) and cause a split brain.
The knobs are also confusing as Bootstrap is an overloaded term. In
BootstrapExpect, we refer to bootstrapping the cluster only after N
servers are connected. But in tests and the knobs above, it refers to
whether the server is a single node cluster and shouldn't wait for any
other server.
Changes
This commit makes two changes:
First, it relies on
BootstrapExpectedinstead ofBootstrapand/orDevModeflags. This change is relatively trivial.Introduce a
Bootstrappedflag to track if the cluster is bootstrapped.This allows us to keep
BootstrapExpectedimmutable. Previously, theflag was a config value but it gets set to 0 after cluster bootstrap
completes.
Notes
@langmartin attempted a different strategy in 2b16278 but resulted into a wackamo game fixing tests.
Fixes #7239